-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
instancetypes: Improve instancetype snapshots #8282
Conversation
/cc @lyarwood |
/area instancetype |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good overall, really like the new ControllerRevision code so thanks for that!
I've listed a few nits and a conflict with a PR I posted earlier today. If you could also add some more context to some of your refactor commits I'd appreciate it. I get that it's boring but it's useful for reviewers and the original author (./me waves) to understand why you wanted to change certain things.
Cheers!
Odd this is also causing the following failure for me locally:
|
Maybe the code is not working yet. I'm still compiling it and uploading to quay to test it. |
Yeah it reproduces in the unit tests as well FWIW. I think we need something like this as TypeMeta isn't present : |
The following is working for me now: https://gist.github.com/lyarwood/1645a40c115eb62e6da37ca1e289be9e |
dbb25d3
to
f32d6a7
Compare
/retest pull-kubevirt-e2e-k8s-1.23-sig-compute |
/retest pull-kubevirt-unit-test |
@akrejcir: The
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@akrejcir: The
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test pull-kubevirt-e2e-k8s-1.23-sig-compute |
/test pull-kubevirt-e2e-k8s-1.23-sig-compute |
f32d6a7
to
3532f80
Compare
/test pull-kubevirt-e2e-k8s-1.23-sig-compute |
3532f80
to
ebeed75
Compare
/test pull-kubevirt-e2e-k8s-1.23-sig-compute |
One letter variables are confusing. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
Move variable definitions to the scope of their usage. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
Changed preference definition in unittests, so that the tests do not require an instancetype. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
One context for instncetype tests and the other for preference tests. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
Changed calls to NewRandomVMIWithEphemeralDisk() to functions from libvmi. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
Small code changes: - renamed variables - unified two Eventually() blocks to one, which checks both conditions Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
Created a new function to store ControllerRevisions. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
The next commits will make changes to the API, so the version needs to be increased. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
Use the new API version instead of v1alpha1. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
The ControllerRevision will store one of these objects: - VirtualMachineInstancetype - VirtualMachineClusterInstancetype - VirtualMachinePreference - VirtualMachineClusterPreference Advantage of this approach is that the objects already contain version information, and they will be properly displayed when using "kubectl get". Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
- Create revision name inside CreateControllerRevision() function. - Lazily initialize logger for errors, because it is needed only when an error happens. - Other smaller code cleanup. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
b960c61
to
1149942
Compare
1149942
to
1baf6c0
Compare
1baf6c0
to
2f4fda4
Compare
/lgtm Apologies for the noise I missed the BUILD.bazel changes, should be good now. |
/retest |
2 similar comments
/retest |
/retest |
/retest-required |
/retest |
@akrejcir: The following tests failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
What this PR does / why we need it:
The
ControllerRevisions
used for instancetype and preference snapshots will contain the whole object.It is useful, because the object contains version information, and we can use methods in the
runtime
package to decode it.Release note: